-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
5095 move onboardingstatus computation from frontend to backend #5954
5095 move onboardingstatus computation from frontend to backend #5954
Conversation
fa8137b
to
86afc6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
- Move onboarding status computation to backend
- Update hooks and states to use new backend-driven onboarding status
- Modify test cases to reflect new onboarding status values
- Remove obsolete frontend onboarding status computation logic
- Introduce
useSetNextOnboardingStatus
hook for consistent status updates
27 file(s) reviewed, 2 comment(s)
packages/twenty-front/src/modules/ui/layout/hooks/useShowAuthModal.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/hooks/useShowAuthModal.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
- Moved
onboardingStatus
computation from frontend to backend - Added logic to
useSetNextOnboardingStatus
- Updated missing redirections in
usePageChangeEffectNavigateLocation
- Simplified conditions in
useShowAuthModal.ts
by removingisDefined
utility function
1 file(s) reviewed, no comment(s)
packages/twenty-front/src/hooks/usePageChangeEffectNavigateLocation.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/onboarding/hooks/useSetNextOnboardingStatus.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/onboarding/hooks/useSetNextOnboardingStatus.ts
Show resolved
Hide resolved
packages/twenty-server/src/modules/workspace-member/repositories/workspace-member.repository.ts
Outdated
Show resolved
Hide resolved
@martmull Looks way better to have this on BE. I feel we should use this opportunity to clarify workspaceStatus from subscriptionStatus from onboardingStatus. Also, I have merge main in your branch but we have some bug because of @magrinj work on typeorm. (userWorkspaceService is using scoped request but it does not seem to be properly instanciated). Could you check with him? |
packages/twenty-server/src/engine/core-modules/onboarding/onboarding.service.ts
Outdated
Show resolved
Hide resolved
f860db2
to
66f4277
Compare
7c34f54
to
a061b65
Compare
…frontend-to-backend
…frontend-to-backend
cd02091
to
dd48784
Compare
dd48784
to
a860990
Compare
a860990
to
5a40bc4
Compare
433fa31
to
9eaa2c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
packages/twenty-front/src/modules/workspace/hooks/useSubscriptionStatus.ts
Outdated
Show resolved
Hide resolved
7eb4159
to
e41d498
Compare
e41d498
to
8ff17d9
Compare
onboardingStatus
computing to server sideuseSetNextOnboardingStatus
usePageChangeEffectNavigateLocation